Output "Overall result" line at the end #71
Conversation
|
Is there anything more you need from me before this can be merged? |
|
It's been a while since I submitted this PR. Haven't heard anything, hope you're ok. Anything else I can do to get this merged? Thanks! |
wolframroesler
force-pushed
the
overall_result
branch
from
a7971a0 to
04e6699
Compare
|
Sorry for the insane delay in my answer and thank you for your contribution and care (I'm ok :) ). I'm not 100% sure I'm OK with this contribution. That's a little bit more of code to maintain and I can't see immediate value to it. Having a failing exit code is good enough for me. I see two use cases: interactive shells or continuous integration. With interactive shell I see more and more people with custom prompt that it would display some hint about the last command having failed, hence giving you the information that some tests have failed. In that case I don't see value for bash_unit displaying that information too. With continuous integration, the fact that bash_unit exit with an error is enough for the continuous integration to fail. Maybe you could think of some other use case I didn't think of. In the meantime, I'm happy with letting this responsibility to an external tool. I guess we would have to take a look also at the tap format to ensure we don't mess with it with this extra text. Regards, |
Before, blocks would be tested in alphabetical order (1, 11, ..., 19, 2, 20, ...) Now they are tested in numerical order (01, 02, ..., 09, 10, ...)
wolframroesler
force-pushed
the
overall_result
branch
2 times, most recently
from
fcabad8 to
0d16051
Compare
Example: ``` $ ./bash_unit tests/test_tap_format Running tests in tests/test_tap_format /home/wolfram/src/bash_unit/tests Running test_assertion_message_is_tap_formatted ... SUCCESS ✓ Running test_bash_unit_accepts_tap_format_option ... SUCCESS ✓ Running test_bash_unit_rejects_invalid_format ... SUCCESS ✓ Running test_multi_lines_assertion_message_is_tap_formatted ... SUCCESS ✓ Running test_tap_format_for_failing_test_with_stdout_stderr_outputs ... SUCCESS ✓ Running test_tap_format_for_one_failing_test ... SUCCESS ✓ Running test_tap_format_for_one_pending_test ... SUCCESS ✓ Running test_tap_format_for_one_succesfull_test ... SUCCESS ✓ Overall result: PASS ✓ ``` So when you've got a lot of tests (more than a screenful) you don't have to scroll up or check $? to tell if any test has failed.
wolframroesler
force-pushed
the
overall_result
branch
from
0d16051 to
4751861
Compare
|
Hi, thanks for your feedback. My use case is indeed interactive (the exit status is good enough fo CI). I have a lot of tests which sometimes output messages that mess up bash_unit's regular output (usually with status messages, but sometimes with error messages, both of which go to the same stream so I can't just ">/dev/null" them). That, plus the fact that the entire test suite is quite large so the whole output doesn't fit in my window even in the best case, makes it quite hard to see if there was an error somewhere. I do have $? in my shell prompt, just wanted something a bit more noticeable (also, not everyone may have the option to modify the shell prompt; I frequently have to live with the default prompt e. g. in docker containers). I agree that the MR changes quite a few lines, but most of these are for the -o option that suppresses the additional output line, and for the documentation/unit tests. The overall complexity is unchanged, and there's no code duplication. |
|
I see your point @wolframroesler and I'm glad you made this pr, thank you. There is an output formatting kind of abstraction in bash_unit. You helped me realize that there is no such hook as notify_suite_succeeded or _ notify_suite_failed_. For instance there exist this function _ notify_suite_starting_ that allows one to customize the message displayed when the test suite starts. But I did not think about customizing the output when the suite finishes, either in success or failure. Know that I can see that, I think I would be glad to solve your issue by introducing these two hooks. Then, for the text format, we would output what you suggested (no need for an option, let's make it systematic as text format is not that formal. And for tap format we would output nothing. I need to make some time to propose you some code with that in mind. |
|
Sounds great, thanks! |
|
Hello @wolframroesler, See my proposal in wolframroesler#1 If you're ok with it, just accept my pull request to update this one and I'll merge the whole thing. Regards, |
|
Will look into it, and thanks for your effort. |
This does not add so much value but is an impediment for the next evolutions.
See https://github.com/pgrange/bash_unit/pull/71/files We add a notify_suites_succeded and notify_suites_failed so that this events may be displayed by any formatter. We use SUCCESS and FAILURE instead of PASS and FAIL just to stick with the lingo used in the rest of bash_unit.
|
Thanks, accepted and merged. Much more elegant than my implementation. |
|
Fixed the conflict. You can go ahead and merge. |
2 participants
Example:
So when you've got a lot of tests (more than a screenful) you don't have to scroll up or check $? to tell if any test has failed.
The "Overall result" line can be disabled with the new
-ooption. This is needed in the test suite which invokes bash_unit recursively, which would result in multiple "Overall result" lines.Also makes a minor change to the test_doc.sh script: Test case names now have two-digit block numbers. Before, blocks would be tested in alphabetical order (1, 11, ..., 19, 2, 20, ...); now they are tested in numerical order (01, 02, ..., 09, 10, ...).